Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Apply all resources in a single kubectl call #872

Merged
merged 19 commits into from
Jan 2, 2018
Merged

Conversation

samb1729
Copy link
Contributor

@samb1729 samb1729 commented Dec 17, 2017

In an attempt to make full-syncs as fast as possible, this change combines all yamels into a big multidoc and attempts to apply that with a single kubectl call.

In the event that the mega-apply fails, fallback to applying yamels one at a time.

Obligatory random cleanups included at no extra cost.

@rade
Copy link
Contributor

rade commented Dec 17, 2017

Does this make flux less tolerant of

  • syntax errors in individual yamls
  • failures in applying a particular yaml

?

@samb1729
Copy link
Contributor Author

samb1729 commented Dec 17, 2017

Yes (to both), fallback yet to be implemented.

I'm trying to figure out if there's some way to get the best of both worlds (single kubectl apply that doesn't stop at the first failure), but if that doesn't work we can always just attempt the mega-apply and go back to applying yamels one at a time if that fails.

@samb1729 samb1729 changed the title [WIP] Apply all resources in a single kubectl call Apply all resources in a single kubectl call Dec 17, 2017
@squaremo
Copy link
Member

Shout-out for the great commit messages ⭐

@squaremo
Copy link
Member

Does this make syncs appreciably faster? It's one exec vs ~100, of course, but I would expect most of the latency to be from the network round-trips kubectl does on each apply.

I'm also worried that there's much more chance of running into the limitation that caused #826 (which will be built into kubectl). It'd be quite easy to accumulate 64k of manifests. Granted, it will fall back to doing things one by one.

@rade
Copy link
Contributor

rade commented Dec 18, 2017

Does this make syncs appreciably faster?

Some measurements would be nice.

@squaremo
Copy link
Member

Probably the best place to get measurements is by running in dev :) We can do that by ignoreing the fluxd deployment, applying a config to use an image built from this branch, and looking at the metrics.

Sam Broughton added 5 commits December 21, 2017 14:09
Since the namespace must be specified in the parsed manifest for us to retrieve
it, we don't need to set this flag. Kubectl figures it out on its own.
These values are never actually used. They look like they are, but their only
usage is immediately reassigned.
This change to the interface facilitates creating an Applier that combines all
yamels into a multidoc to be applied in one call to kubectl.

Note that the deleted test was testing behaviour that exists nowhere in flux.
All references to `SyncAction` in the code contain only a single change, so the
test was unnecessary.
Since the namespace flag is no longer needed for kubectl, and since the tests
don't actually need the object name, we can pass a byte slice and simplify a
bit.

This change guides us towards doing the single mega-apply without smushing bytes
into an apiObject in a kludgey way.
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebut or adapt at your discretion.

return "default"
}
return obj.Metadata.Namespace
func (o *apiObject) hasDefaultNamespace() bool {

This comment was marked as abuse.

This comment was marked as abuse.

version string // string response for the version command.
logger log.Logger
sshKeyRing ssh.KeyRing

sync.Mutex

This comment was marked as abuse.

This comment was marked as abuse.

cmd.Stderr = c.stderr
return cmd
func (c *Kubectl) execute(logger log.Logger, errs cluster.SyncError) {
defer c.changeSet.clear()

This comment was marked as abuse.

This comment was marked as abuse.

}
}

type executeSet struct {

This comment was marked as abuse.

This comment was marked as abuse.

@@ -146,7 +146,7 @@ func main() {
var clusterVersion string
var sshKeyRing ssh.KeyRing
var k8s cluster.Cluster
var image_creds func() registry.ImageCreds
var imageCreds func() registry.ImageCreds

This comment was marked as abuse.

This comment was marked as abuse.

return
}
// Attempt to apply everything in s at once, else fallback to applying one at a time.
if err := c.doCommand(logger, s.rw, s.cmd...); err != nil {

This comment was marked as abuse.

set.stage(obj)
}

c.exec(logger, defaultSet, cmd, errs)

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

Sam Broughton added 9 commits December 22, 2017 16:38
As much as I love finding places to put channels, in this case it is entirely
unnecessary. The only use of actionc can be replaced with a mutex, and any
future methods can just compete for the lock instead of competing for a channel
write.
This brings back the previous ordering without doing a sort. Since we no longer
apply objects one at a time, we don't need an id.
This command was simply testing the mock.
We have this enormous block of nothingness all for satisying the requirements of
a constructor we don't even need to use.
In some not-so-well-defined cases, applying an object to the default namespace
can fail when its yaml doesn't explicitly specify the namespace. The simplest
way to overcome this is to separate them out and pass the namespace flag to
kubectl.
Sam Broughton added 2 commits December 28, 2017 18:25
The ordering of application/deletion of namespaces before/after namespaced
resources is specific to kubernetes and so does not belong in the generic sync
package. This ordering is now implicitly handled by the order in which commands
are run in the kubernetes package.
It occurred to me that there's no good reason this state should be stored within
the struct that happens to implement the Applier interface. The state can be
built in the Sync method and then garbage collected at the end.
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for persevering Sam.

}
return <-errc
c.applier.apply(logger, cs, errs)

This comment was marked as abuse.

This comment was marked as abuse.

@samb1729 samb1729 merged commit 0972ee9 into master Jan 2, 2018
@samb1729 samb1729 deleted the kubectl-megapply branch January 2, 2018 13:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants